contrib/gin-gonic/gin: Extract parent span info from HTTP Headers #279
Conversation
There was a problem hiding this comment.
Did you read our contributing guidelines? Generally you should open an issue before committing to any work.
contrib/gin-gonic/gin/gintrace.go
Outdated
| @@ -16,7 +16,12 @@ import ( | |||
| func Middleware(service string) gin.HandlerFunc { | |||
| return func(c *gin.Context) { | |||
| resource := c.HandlerName() | |||
|
|
|||
There was a problem hiding this comment.
Remove this empty line please.
contrib/gin-gonic/gin/gintrace.go
Outdated
| @@ -16,7 +16,12 @@ import ( | |||
| func Middleware(service string) gin.HandlerFunc { | |||
| return func(c *gin.Context) { | |||
| resource := c.HandlerName() | |||
|
|
|||
| // try to get a span context from request headers | |||
There was a problem hiding this comment.
Please remove this comment, it doesn't add anything.
contrib/gin-gonic/gin/gintrace.go
Outdated
| @@ -16,7 +16,12 @@ import ( | |||
| func Middleware(service string) gin.HandlerFunc { | |||
| return func(c *gin.Context) { | |||
| resource := c.HandlerName() | |||
|
|
|||
| // try to get a span context from request headers | |||
| sctx, _ := tracer.Extract(tracer.HTTPHeadersCarrier(c.Request.Header)) | |||
There was a problem hiding this comment.
Don't ignore the error, it is there for a reason. You can get inspired from here on how to accomplish this: https://github.com/DataDog/dd-trace-go/blob/v1/contrib/internal/httputil/trace.go#L25-L27
contrib/gin-gonic/gin/gintrace.go
Outdated
|
|
||
| // try to get a span context from request headers | ||
| sctx, _ := tracer.Extract(tracer.HTTPHeadersCarrier(c.Request.Header)) | ||
|
|
There was a problem hiding this comment.
Remove this empty line too please.
|
Thanks @gbbr for the review. Sorry for not respecting the guidelines, I read them too quickly... Thanks |
|
Yes, please always open an issue and link to it from the PR. It helps us keep track of change logs and manage things better. Many thanks! |
|
FIxes #280 |
|
Thanks. It's looking good. Will do another review soon. |
| @@ -167,3 +167,27 @@ func TestGetSpanNotInstrumented(t *testing.T) { | |||
| response := w.Result() | |||
| assert.Equal(response.StatusCode, 200) | |||
| } | |||
|
|
|||
| func TestExtractSpan(t *testing.T) { | |||
There was a problem hiding this comment.
Can we please call this TestPropagation? "Propagation" is the terminology we use throughout and will make it more obvious.
| span, ok := tracer.SpanFromContext(c.Request.Context()) | ||
| assert.True(ok) | ||
|
|
||
| //Ensure current span if a child of the span injected in the HTTP request |
There was a problem hiding this comment.
Please remove this comment and the empty line above it.
| }) | ||
|
|
||
| router.ServeHTTP(w, r) | ||
| } |
There was a problem hiding this comment.
missing a newline, should be fixed now
Use propagation features to try to get some span context in the incoming request headers. If the headers are not set, the comportment is the same as before If a span is already present in the request's context, it will be used primarily as the parent span. Fixes DataDog#280
|
Thank you |
…taDog#279) Use propagation features to try to get some span context in the incoming request headers. If the headers are not set, the comportment is the same as before If a span is already present in the request's context, it will be used primarily as the parent span. Fixes DataDog#280
Use propagation features to try to get some span context in the incoming
request headers.
If the headers are not set, the comportment is the same as before
If a span is already present in the request's context, it will be
used primarily as the parent span.